Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bump device-plugin-manager and kubelet deps #30

Merged
merged 2 commits into from
Sep 25, 2020
Merged

Bump device-plugin-manager and kubelet deps #30

merged 2 commits into from
Sep 25, 2020

Conversation

rhrazdil
Copy link
Contributor

@rhrazdil rhrazdil commented Sep 17, 2020 •
edited
Loading

Signed-off-by: Radim Hrazdil [email protected]

What this PR does / why we need it:
A followup to ubruptly merged PR #29

Printing clusternetworkaddons CR when cluster-up fails to provide debug info.
Bumping last dependencies device-plugin-manager and k8s.io/kubelet to v0.19.1
New kubelet version requires dpm.PluginInterface to implement GetPreferredAllocation function.

Added implementation returns nil as a list of preffered devices, which should keep the original behaviour [0]

[0] https://github.com/kubernetes/kubernetes/pull/92665/files#diff-b54ee8195220ea88e59b6eb423e83dd1R741

Special notes for your reviewer:

Release note

Bump device-plugin-manager to v0.19.2
Bump kubelet to v0.19.2

@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 17, 2020
phoracek
phoracek requested changes Sep 17, 2020
View reviewed changes
cluster/up.sh Outdated
@@ -5,7 +5,7 @@ set -ex
source ./cluster/kubevirtci.sh
kubevirtci::install

$(kubevirtci::path)/cluster-up/up.sh
$(kubevirtci::path)/cluster-up/up.sh || $(kubevirtci::path)/cluster-up/kubectl.sh get networkaddonsconfig cluster -o yaml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "print" would silence the error here. So setup would continue even after we failed to set it up.

I also think that we could keep the print on the problematic command - on the wait. I don't think it would make sense for other issues that can happen inside "up"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, you're correct about the error being silenced...

As I mentioned before, the wait command does not reside in this repository, it's one of the first two in the following output:

~/Sources/macvtap-cni # grep -R kubectl | grep wait 
**_kubevirtci/cluster-provision/k8s-multus/scripts/node01.sh:kubectl --kubeconfig=/etc/kubernetes/admin.conf wait networkaddonsconfig cluster --for condition=Ready --timeout=800s**
**_kubevirtci/cluster-provision/k8s-multus/scripts/provision.sh:kubectl --kubeconfig=/etc/kubernetes/admin.conf wait networkaddonsconfig cluster --for condition=Ready --timeout=800s**
_kubevirtci/cluster-up/cluster/kind/common.sh:    _kubectl wait -n kube-system --timeout=12m --for=condition=Ready -l k8s-app=kube-dns pods
_kubevirtci/cluster-up/cluster/kind/common.sh:    _kubectl wait --for=condition=Ready pod --all -n kube-system --timeout 12m
_kubevirtci/cluster-up/cluster/kind-k8s-sriov-1.14.2/config_sriov.sh:# not using kubectl wait since with the sriov operator the pods get restarted a couple of times and this is

It's being run from _kubevirtci witch is git cloned at cluster/up.sh runtime.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I'm so sorry for missing that. I though we are creating CNAO here directly. My bad.

In that case, the debug output should go to https://github.com/kubevirt/kubevirtci

Could we limit this PR only to the bump.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, removed that commit from this PR

@maiqueb
Copy link
Collaborator

maiqueb commented Sep 17, 2020

/hold

Need to take a look at this; we've been too trigger happy in this repo in the past.

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 17, 2020
@rhrazdil rhrazdil force-pushed the print_nac_if_up_fails branch from 7b0ac30 to edc2bfb Compare September 18, 2020 08:35
@rhrazdil rhrazdil changed the title Printing clusternetworkaddons cr when cluster-up fails and bump kubelet Bump device-plugin-manager and kubelet deps Sep 18, 2020
@phoracek
Copy link
Member

/lgtm

Thanks!

Let's hear from Mr. @maiqueb

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2020
maiqueb
maiqueb requested changes Sep 21, 2020
View reviewed changes
Copy link
Collaborator

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please correct the commit msg of the second commit.

@rhrazdil rhrazdil force-pushed the print_nac_if_up_fails branch from edc2bfb to cf8c15c Compare September 21, 2020 15:41
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2020
@rhrazdil rhrazdil force-pushed the print_nac_if_up_fails branch 2 times, most recently from 812d345 to 792e1cd Compare September 21, 2020 16:32
Radim Hrazdil added 2 commits September 21, 2020 18:33
Bump device-plugin-manager and kubelet
b4a8326 
Bump Device Plugin Manager and kubelet. New kubelet version requires
dpm.PluginInterface to implement GetPreferredAllocation function.

Signed-off-by: Radim Hrazdil <[email protected]>
make vendor
83d466c 
Run make vendor to update vendored packages.

Signed-off-by: Radim Hrazdil <[email protected]>
@rhrazdil rhrazdil force-pushed the print_nac_if_up_fails branch from 792e1cd to 83d466c Compare September 21, 2020 16:34
maiqueb
maiqueb reviewed Sep 22, 2020
View reviewed changes
Copy link
Collaborator

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Thanks. You can remove the hold when you feel this is good enough.

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2020
@rhrazdil
Copy link
Contributor Author

Thanks
/hold cancel

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2020
@phoracek
Copy link
Member

/approve

@kubevirt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: phoracek, rhrazdil

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Sep 25, 2020
@kubevirt-bot kubevirt-bot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 25, 2020
@kubevirt-bot kubevirt-bot merged commit 6a92a3b into kubevirt:master Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maiqueb maiqueb maiqueb requested changes

@phoracek phoracek phoracek requested changes

Assignees

@phoracek phoracek

@maiqueb maiqueb

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rhrazdil @maiqueb @phoracek @kubevirt-bot